[crypto,devbundle] Package cryptolib as part of the release devbundle (+ doxygen fixes)#30712
[crypto,devbundle] Package cryptolib as part of the release devbundle (+ doxygen fixes)#30712AlexJones0 wants to merge 6 commits into
Conversation
AlexJones0
force-pushed
the
cryptolib_devbundle
branch
from
4074d3f to
b0a0ec4
Compare
luismarques
left a comment
luismarques
left a comment
There was a problem hiding this comment.
This PR is still marked as Draft but it already LGTM. We can discuss the open questions listed in the PR summary in the Wed meeting and the SW WG, but I think that such changes can always be made later.
Fixes a variety of Doxygen issues due to function header comments on function prototypes in Cryptolib's include headers that have fallen out- of-sync with their corresponding function prototypes. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Comments on struct fields / enumerated values should be prefixed by '///' instead of '//'. Also, the '<>' syntax should not be used without escapes inside a Doxygen comment. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Suppress remaining Doxygen warnings for cryptolib by adding documentation to undocumented fields. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
AlexJones0
force-pushed
the
cryptolib_devbundle
branch
2 times, most recently
from
e1a46d3 to
9fe88fc
Compare
AlexJones0
force-pushed
the
cryptolib_devbundle
branch
2 times, most recently
from
7a6f20a to
7849e15
Compare
When cryptolib is consumed from a tarball (either the cryptolib pkg or upcoming devbundle), we will no longer have the same sw/device/lib/crypto relative path - so we should not encode it, if we want to be able to use these headers externally. All relative locations already include the files properly, so flatten this path. Likewise, flatten the path to the "datatypes.h" header include in the export test - since this makes more sense to how we actually intend to use the exported files. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
AlexJones0
force-pushed
the
cryptolib_devbundle
branch
from
7849e15 to
d7be32a
Compare
siemen11
left a comment
siemen11
left a comment
There was a problem hiding this comment.
Thank you @AlexJones0! These are absolutely nice changes! Thank you for cleaning up the doxygen for the cryptolib and to handle our build options 🙏 this is really appreciated!
For your open question, indeed it is up to our side to find out which options are wanted. I think for now what you propose is good and we can later make a change adding or changing the option if we find what our users want
| exports_files(glob(["*.h"])) | ||
|
|
||
| # Header defining build options for the public ABI | ||
| cryptolib_api_config_header( |
There was a problem hiding this comment.
A question: we also have our FIPS build, for example, --config=crypto_fips_all. Is that something we could place in api_config as well? In fact, we have several build options https://github.com/lowRISC/opentitan/blob/master/doc/security/cryptolib/cryptolib_api.md#build-configurations.
You do not have to implement it btw, but it would be nice to know what you think because the changes so far look really nice, I would then propagate what you did so far with our other build options
There was a problem hiding this comment.
Sure, I don't see a reason why we can't place more things in here. The reason that I only put this define in here for now is because it is the only macro I could see that potentially changes the ABI, and could cause issues if somebody used the library incorrectly. For defines that only impact the internals of the implementation, there was no need to expose these.
But it might be nice to expose the build options in this way so that users can consume them as well - I think you probably have a better grasp of what would be good to put in here or not, so if you have any suggestions I'm happy to make them (or we can add them in a follow-up PR). It should be quite easy to change the rule to put more stuff here.
There was a problem hiding this comment.
Indeed, but no worries, I can add those as well. Just wanted to ask your opinion :D
| prefix = "lib", | ||
| ) | ||
|
|
||
| pkg_filegroup( |
There was a problem hiding this comment.
On our side, we should also come with the BUILD options for the cryptolib to enter into the release bundle... I believe the vanilla option (no flags) is perfect for now. We will need a FIPS build later, but I need to sit with stakeholders for that. When that shakes out, I can make a PR and put you as reviewer where we try add this FIPS option as well
The ABI of cryptolib potentially changes depending on whether the `OTCRYPTO_DISABLE_BUF_INTEGRITY_CHECKS` macro is defined or not. When we're exporting cryptolib for external consumption, we need to be careful to avoid the case where cryptolib is compiled with this feature undefined (i.e. the integrity checks are enabled at runtime), but where a user might try to use this define (to reduce code size / footprint), leading to undefined behaviour. So, rather than exposing this compile-time gate as a #define through Bazel targets (based on some config setting), create a generated `api_config.h` header file which contains the option that cryptolib was compiled with. This way the option is not exposed to downstream users, and we correctly capture the configuration that cryptolib was compiled with (as you might, for example, want to build cryptolib with this feature disabled, which is not the default option). Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
There is some additional packaging on top of that already present in cryptolib to re-arrange the files to fit the more conventional format for a C static library to link against. These are localised to the devbundle and should be easy to change if necessary. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
AlexJones0
force-pushed
the
cryptolib_devbundle
branch
from
d7be32a to
dc04f68
Compare
3 participants
Addresses #29778.
This PR build and releases cryptolib as part of the dev-bundle. This is primarily done using the existing packaged cryptolib targets. Specifically, this PR:
OTCRYPTO_DISABLE_BUF_INTEGRITY_CHECKSgate from being a Bazel-onlycc_librarydefine to its own generatedapi_config.hheader file, which is then used by the other headers. This ensures that the bundled version of cryptolib enforce that the header cannot be misused in a way that might introduce ABI incompatibility with the archive.include/otlibcrypto/*.handlib/otlibcrypto.a). This is quite subjective and easy to change, so please mention if you think there should be a different structure or naming scheme here.Use of these exported packages are already tested through
//sw/device/tests/crypto:otcrypto_export_test_fpga_cw340_sival_rom_ext, which I've tested both with and without--define disable_buf_integrity_checks=true. I've done a more rigorous test locally here - though note that this is hardcoding local paths in the Bazel lockfile, and so is not portable.A couple of other open questions here: